Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Probably too much #29

Merged
merged 7 commits into from
Jan 11, 2024
Merged

Probably too much #29

merged 7 commits into from
Jan 11, 2024

Conversation

mistermoe
Copy link
Member

What's Changed

Implemented DID URL Dereferencing


Implemented JWS signing and verification


Implemented JWT verification

Not all that happy with Jwt.dart right now for the reasons described here. will tackle that in another PR


Reorganized web5 package to reduce the amount of import statements that we end up with in any given file.

@wesbillman and i chatted about this and decided it'd be interesting to get a feel for what it'd be like if we introduced rollup files for directories where the rollup file exports everything we want to export out of any given directory.

so for example, instead of having to import n thangs out of the crypto directory, you can now. just plop a import 'package:web5/src/crypto.dart'; in and get it all.

Selective importing are still possible using show, hide, or continuing to import each file individually.


reorganized web5/dids directory.

the dids directory in main felt pretty noisy and it wasn't exactly clear what functionality the directory should showcase on initial glance.

Concretely,

  • moved all data model classes into a data_models dir.
  • created a directory for each did method e.g did_jwk, did_dht
  • moved dns_packet into did_dht because it's only relevant to did_dht

i think this potentially does a better job of highlighting the most important aspects of that directory but interested to hear other opinions.

Note

eventually. might be worth moving dns_packet into its own individual package/repo and publishing it. need tests and shiz for that tho

Comment on lines +1 to +9
export './crypto/jwk.dart';
export './crypto/dsa.dart';
export './crypto/ed25519.dart';
export './crypto/dsa_name.dart';
export './crypto/dsa_alias.dart';
export './crypto/secp256k1.dart';
export './crypto/key_manager.dart';
export './crypto/dsa_algorithms.dart';
export './crypto/in_memory_key_manager.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

return resource;
}

resource = service?.firstWhere((vm) => idVariations.contains(vm.id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a firstOrNullWhere as well if it's possible vm.id is not found. With this code, it will throw if it's not found. With firstOrNullWhere it will just return null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice! good call. didn't realize the collections package had fancy extensions.

done! 0c9dda6

Comment on lines 1 to 3
import 'package:web5/src/dids/data_models/did_document.dart';
import 'package:web5/src/dids/data_models/did_document_metadata.dart';
import 'package:web5/src/dids/data_models/resolution_metadata.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_models feels like a strange name, but maybe it makes sense in this context. Would just models work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally agreed. changed to structures: f358533

Comment on lines 31 to 35
if (resource != null) {
return DidDereferenceResult(contentStream: resource);
} else {
return DidDereferenceResult.withError('notFound');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit.

return resource != null ? 
  DidDereferenceResult(contentStream: resource) :
  DidDereferenceResult.withError('notFound');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it. changing now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: fa5dbae

);
}

if (header.typ == null || header.typ != 'JWT') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this case sensitive? Do we want/need to support jwt as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great question. one sec checking the JWT RFC to see what it says:

image

kk looks like we should support lowercase as well since uppercase is simply RECOMMENDED and not REQUIRED

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done! 97b8889

@wesbillman
Copy link
Member

Woot! This is awesome. In the future it would be nice to separate the restructure PR from the new stuff PR but these are early days so there are no rules!!! :)

@mistermoe mistermoe merged commit 51f8a10 into main Jan 11, 2024
1 check passed
@mistermoe mistermoe deleted the jws branch January 11, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants